Skip to content

BUG: make a failing Monte Carlo worker say so - #1182

Open
thc1006 wants to merge 10 commits into
RocketPy-Team:developfrom
thc1006:bug/report-a-worker-that-fails-before-its-first-simulation
Open

BUG: make a failing Monte Carlo worker say so#1182
thc1006 wants to merge 10 commits into
RocketPy-Team:developfrom
thc1006:bug/report-a-worker-that-fails-before-its-first-simulation

Conversation

@thc1006

@thc1006 thc1006 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Six ways a failing Monte Carlo worker got away without saying so. Three of them came out of review of the first three, which is why this is longer than it started.

Stacked on #1181, which is the parallel run being unable to start at all. This branch carries that commit, so the diff here shows it too; the work of this pull request is the two commits on top. Happy to rebase once the other one lands.

Pull request type

  • Code changes (bugfix, features)

Checklist

Current behavior

A worker that fails early dies inside its own handler. __sim_producer binds sim_idx and inputs_json inside the simulation loop:

while sim_monitor.keep_simulating():
    sim_idx = sim_monitor.increment() - 1
    inputs_json, outputs_json = "", ""

and its except block reads both. A worker that fails in the seeding above the loop, or in the claim that opens it, arrives there with neither name assigned:

UnboundLocalError: cannot access local variable 'inputs_json'

That happens after mutex.acquire() and before mutex.release(), so a manager lock is left held by a process that no longer exists. The next worker blocks on acquire() and the run waits forever. error_event.set() is the handler's last line, so the parent is never told either.

A worker that is killed is not noticed at all. The parent joins each worker and reads the event. A signal runs no handler, so the event stays clear and the join returns because the process is gone. With a SIGKILL in the second of six simulations across two workers:

simulate() returned normally
asked for 6, rows on disk: 2

I would rather have the hang than that one. A hang is at least visible.

New behavior

Both names are bound before the try, and the message says worker startup when no index was claimed instead of naming one that does not exist. The handler now finishes, releases the mutex and sets the event, so a worker failure surfaces as a RuntimeError naming the error file rather than as a wait.

After the join, _refuse_a_worker_that_did_not_finish reads the exit codes. Anything other than zero is refused. It sits before the event check because the two are disjoint: a worker that reports through its handler exits cleanly, and one that was killed only leaves its exit code behind.

The handler holds the shared mutex while it reports, so a failure in the reporting used to leave the lock held by a process that had already gone. The event is set first and from outside the lock, the lock is released from a finally, and each reporting step is separate so an unwritable log cannot replace the failure being reported. A startup failure writes a row of its own now, since the caller is told to read that file and a traceback a worker printed is not there once its output is redirected.

The lock belongs to the manager and is not released when its holder is killed, so a sibling can block on a lock nobody owns while an unbounded join waits with it. That is the case the exit-code check exists for and the one it could not see. _join_the_workers polls instead, and acts only once a worker has actually ended badly. A run that is merely slow is never bounded: an exit code, not a duration, decides. This is deliberately not a general worker-lifecycle change; it is the smallest thing that makes the check above reachable.

Scope is the parallel producer. __run_in_serial has the same unbound inputs_json and belongs to #1177, whose tests cover it already; I have deliberately not touched it.

Breaking change

  • No

A run that used to hang now raises, and a run that used to return with missing rows now raises. Both were already failures.

Additional information

Verification, on a clean tree:

pytest tests/unit                     2211 passed
pytest rocketpy --doctest-modules       48 passed
pytest tests/integration               154 passed
pytest tests/acceptance                 18 passed
ruff check . / ruff format --check .   clean
pylint rocketpy/ tests/ docs/          10.00/10, exit 0

The four tests/unit/test_sensitivity.py failures on my machine are a missing statsmodels and fail the same way on an untouched develop.

Mutations, each leaving a control standing:

undone goes red still passes
the two names are not bound before the try five the test where an index was claimed first
only inputs_json is bound, which is what the traceback points at still five same one
the parent stops reading exit codes the killed-worker test the rest
if process.exitcode in place of != 0 the None case the rest
the reporting is not wrapped in try/finally the three lock tests the rest
the join goes back to being unbounded all eight join tests the rest
the join acts on how long a worker took rather than on how it ended exactly one, the slow-run test the rest
the join stops reading the event the reported-failure test the other four files
the outer handler joins unbounded again the syntax guard the other four files
the run is judged on exit codes alone the zero-exit test the other four files

Two rows there are worth pointing at. Fixing only the name in the traceback looks complete and leaves the message raising on the other one. And deciding the join on elapsed time rather than on an exit code passes everything except the test that says a slow run must be left alone, which is the failure mode that would hurt real users most.

Three more, each of which let a broken run still look finished. They were raised in review of the commits above and all three reproduced.

The join waited only on exit status. A worker that fails the ordinary way is caught by the producer, reports through the event and returns, so it exits cleanly. With a sibling stuck, the parent saw one clean exit and one live process and never stopped. A reported failure ends the wait now too. A manager that cannot be asked is not taken as evidence either way, which has its own control test.

The bounded shutdown was undone one level up. After the exit-code check raised, the outer handler joined every process again with no timeout, so the stubborn worker the bound exists for was waited on anyway. That handler uses the same teardown now, and a test walks the parallel path's syntax to keep an unbounded join out of it.

An exit code says how a process ended, never whether the index it claimed reached the logs. The monitor counts claims, not rows, so it cannot stand in either. Measured, six simulations across two workers leaving through os._exit(0):

before   simulate(): returned normally     rows on disk: 0
after    RuntimeError: The run is incomplete: the input log does not hold
         every simulation that was asked for, 6 of 6 are missing

The run is checked against its own logs at the end: both must hold exactly the simulations asked for, none twice, none unreadable, none numbered past the run.

Still not covered, and I would rather say so than imply otherwise: a worker killed between mutex.acquire() and the write it is guarding leaves a torn record behind. The run is stopped and reported now, but the rows it was midway through are not repaired.

_refuse_a_worker_that_did_not_finish is a module-level function rather than a method because the run paths are driven by stub objects in the tests, where self.__helper() would not resolve.

The stand-in worker in the join tests gives up after two hundred polls. Reproducing a real hang there would take a CI job down with it rather than report, and requirements-tests.txt has no pytest-timeout; I found that out by hanging my own mutation run for the full fifteen minutes.

The Monte Carlo objects in these tests are built on tmp_path rather than retargeted after construction. filename is a plain attribute and the three log paths are settled in __init__, so assigning it leaves a test writing the fixture's own files into the working directory, which is what these were doing.

Merged with #1169, #1170 and #1171's follow-up into a throwaway tree on develop and run there too. The fixed-seed baseline is unchanged across all of it: stochastic_calisto under seed 42 reads mass=14.906007947 radius=0.063501935.

@thc1006
thc1006 requested a review from a team as a code owner August 17, 2026 19:14
@thc1006
thc1006 force-pushed the bug/report-a-worker-that-fails-before-its-first-simulation branch from fa81b7f to 65a2da2 Compare August 17, 2026 19:17
@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.79%. Comparing base (7e785a6) to head (629cd87).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1182      +/-   ##
===========================================
+ Coverage    89.96%   90.79%   +0.82%     
===========================================
  Files          131      131              
  Lines        17527    17608      +81     
===========================================
+ Hits         15769    15987     +218     
+ Misses        1758     1621     -137     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thc1006
thc1006 force-pushed the bug/report-a-worker-that-fails-before-its-first-simulation branch 3 times, most recently from 6b260bb to 2adc9d4 Compare August 17, 2026 21:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes parallel Monte Carlo seed handling and ensures failed workers cannot silently hang or produce incomplete successful runs.

Changes:

  • Normalizes SeedSequence values without collapsing worker streams.
  • Adds worker failure reporting, exit-code validation, and bounded shutdown polling.
  • Adds regression coverage for parallel execution and worker failure modes.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
rocketpy/tools.py Adds deterministic SeedSequence conversion.
rocketpy/stochastic/stochastic_model.py Accepts worker seed sequences.
rocketpy/simulation/monte_carlo.py Improves worker reporting and lifecycle handling.
tests/unit/stochastic/test_seed_types.py Tests seed compatibility and stream independence.
tests/unit/simulation/test_monte_carlo_parallel_runs.py Exercises real serial and parallel runs.
tests/unit/simulation/test_monte_carlo_worker_reporting.py Tests worker failure diagnostics.
tests/unit/simulation/test_monte_carlo_worker_join.py Tests polling and shutdown behavior.
tests/unit/simulation/test_monte_carlo_worker_exit.py Tests abnormal worker exit detection.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

# Bound before the try: the handler below reads both, and a failure in
# the seeding, or in the claim that opens the loop, reaches it with
# neither of them assigned.
sim_idx, inputs_json = None, ""
Comment thread rocketpy/simulation/monte_carlo.py Outdated
Comment on lines 594 to 595
with suppress(Exception):
error_event.set()
@thc1006

thc1006 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the approve, @Gui-FernandesBR. Flagging that the branch has moved a fair way since you looked at it, so you can decide whether you want another pass.

TL;DR: four commits landed after your approve (6fb180f4 to cad1281a, +497/-33). One of them undoes a change an earlier round of feedback asked for, because measuring it showed it made things worse. This repo does not dismiss a review on push, so the PR is sitting mergeable with those four unreviewed.

What landed, and why:

  • f5b992c7: a worker that exits 0 without recording the index it claimed was treated as a success. Six simulations across two workers leaving through os._exit(0) returned normally with nothing written. The run is now checked against its own logs at the end.
  • 8962cc52: the failure report named the wrong simulation. A worker kept sim_idx and inputs_json after committing a row, so a failure in the next claim was blamed on the simulation that had just succeeded, and that simulation's inputs were written to the error log as well. Also: shutdown shares one deadline across the fleet instead of one per worker, uses a monotonic clock, and kills what outlives terminate.
  • c9a8a9d7: a blank line in a log, which is what an interrupted write leaves.
  • cad1281a: a data_collector key called index replaced the number of the simulation the row belonged to, because the collector's values are merged over the record after the index is written. A two-simulation run with such a collector wrote both rows as 999. Refused now where the collector is handed over.

The part worth your eye is the reversal. I had made a reported failure end the parent's wait, which reads sensibly and turns out to be wrong: a worker that reports has left nothing behind, and its siblings stop on their own once they finish the simulation in hand. With the event treated as a reason to stop, a sibling that needed forty polls was terminated after three. It was not stuck, it was mid-simulation, and it would have exited cleanly with its rows intact. Only a worker that died can hold the shared lock for good, so only that ends the wait now. There is a test pinning it, and reverting the line turns that test red.

One thing I have not covered and would rather say than imply: the monotonic clock is a judgement call, not a tested one. Pinning it would mean moving the system clock, which I do not think belongs in CI.

Nothing here needs action from you if you are happy with it as is. If you would rather the extra work went somewhere separate, I am glad to move those four out into their own PR and put this back to what you approved.

Apologies for the moving target: I kept finding things while reviewing my own answer to the last round, and pushed each as I went rather than waiting. It is settled now.

A parallel run spawns a SeedSequence per worker and passes it to
environment, rocket and flight. _sampler_seed then fed it to
SeedSequence(entropy=...), which takes an int or a sequence of ints,
so the first worker raised TypeError before drawing anything.

The call was reached only from the custom sampler reset until RocketPy-Team#1117
added the list-choice generator, which every model goes through. A real
two-worker run passes at d21abde^ in 2.32s and does not finish on
develop: the worker's own error path raises UnboundLocalError on
inputs_json, so the parent never learns it died and the run hangs.

The children of one root share their entropy and differ by spawn_key,
so the value is folded through generate_state rather than read off
entropy, which would put every worker on one sampler stream. Nothing
is consumed, and an int or None seed keeps the stream it had.

The fold lives in rocketpy.tools, since the component streams and the
per-index seeding both need the same one and three copies would drift
on width and word order. _sampler_seed does its own final fold through
it as well rather than repeating the four lines.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
__sim_producer binds sim_idx and inputs_json inside the loop, and its
handler reads both. A worker that fails in the seeding above the loop,
or in the claim that opens it, reached the handler with neither name
assigned and died with UnboundLocalError instead of recording anything.

The parent learns a worker failed from error_event, which the handler
sets on its last line, so it was never reached either and the run
waited rather than stopping.

Both names are bound before the try now, and the message says worker
startup when no index was claimed rather than naming one that does not
exist. Binding only the name in the traceback is not enough: the
message then raises on the other one, which the tests cover.

Scoped to the parallel producer. The serial handler has the same
unbound inputs_json and is RocketPy-Team#1177's.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The workers say they failed by setting an event, and the parent joins
them and reads it. A worker that is killed runs no handler, so the
event stays clear, the join returns because the process is gone, and
the run reports the simulations it never wrote as done.

Measured with a worker leaving in the second simulation of six, two
workers: simulate() returned normally with two rows on disk.

Its exit code is what is left of a worker that ends this way, so the
parent reads that too. Anything other than zero is refused, None
included, since that is a worker that has not finished at all.

The handler around it holds the manager mutex while it reports, so a
failure in the reporting left the lock held by a process that had
already gone and the next worker waited on it. The event is set first
and outside the lock, the lock is released from a finally, and each
reporting step is separate so an unwritable log cannot replace the
failure being reported. A startup failure writes a row of its own now
rather than nothing, since the caller is told to read that file.

The test leaves through the data collector rather than a patched
method, since a spawn platform re-imports the module in the child and
never sees the patch, and through os._exit rather than a signal, since
SIGKILL is POSIX-only. Checked on both start methods. The Monte Carlo
objects are built on tmp_path rather than retargeted, because filename
is a plain attribute and the three log paths are set in __init__.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The lock the workers share belongs to the manager and is not released
when the process holding it is killed. A sibling then blocks on a lock
nobody owns, and the parent, joining without a timeout, waits with it.
The exit code check the previous commit added is never reached, so the
one case it exists for is the one it cannot see.

The join polls now, and acts only when a worker has actually ended
badly. A run that is merely slow is never bounded: an exit code, not a
duration, is what says a worker is gone. The survivors are asked
through the event first, since one between simulations leaves with its
logs intact, and only the ones still running after that are ended.

Undoing this leaves every test in the new file red. Deciding on how
long a worker has taken instead of on how it ended leaves exactly one
red, which is the test that says a slow run must be left alone.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Lifted out of __sim_producer unchanged. The producer was over pylint's
statement limit once the per-index seeding shortens it elsewhere, and
the handler is one thing rather than part of the loop around it.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Three ways a failed run still passed for a finished one, all of them
found by review of the previous commits here.

The join waited only on exit status. A worker that fails the ordinary
way is caught by the producer, reports through the event and returns,
so it exits cleanly. With a sibling stuck, the parent saw one clean
exit and one live process and never stopped. A reported failure ends
the wait now as well, and a manager that cannot be asked is not taken
as evidence either way.

The bounded shutdown was undone one level up: after the exit-code
check raised, the outer handler joined every process again with no
timeout, so the stubborn worker it exists for was waited on anyway.
That handler uses the same bounded teardown now, and a test walks the
parallel path's syntax to keep an unbounded join out of it.

An exit code says how a process ended, never whether the index it had
claimed reached the logs, and the monitor counts claims rather than
rows. Measured, six simulations across two workers leaving through
os._exit(0): simulate() returned normally with nothing written. The
run is checked against the logs themselves at the end now. Both must
hold exactly the simulations asked for, none twice, none unreadable,
none numbered past the run.

The stand-in worker in the join tests gives up after two hundred
polls. Reproducing a real hang there would take a CI job down with it
rather than report, and requirements-tests.txt has no pytest-timeout.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Review of the previous commits found three things, and reviewing my own
answer to the first of them found a fourth.

A worker kept sim_idx and inputs_json after committing a row, so a
failure in the claim that opened the next round was reported against
the simulation that had just succeeded, and that simulation's inputs
were written to the error log as well. Measured: output held index 0
and the error log held index 0 with the same inputs. They are cleared
once a row is committed, since nothing is in flight between two
simulations.

A worker whose event could not be set returned anyway, so it exited
cleanly and the parent saw neither an event nor a bad exit. It re-
raises when it could not announce, leaving the exit code to say so.

The bounded shutdown deserved more than one stage. It shares one
deadline across the fleet rather than giving each worker its own, uses
a monotonic clock so a correction cannot move a shutdown already under
way, and kills what outlives terminate.

The fourth is mine. I had made a reported failure end the wait, which
sounded right and was not: a worker that merely reports leaves nothing
behind, and its siblings stop of their own accord once they finish the
simulation in hand. Measured, a sibling that needed forty polls was
terminated after three. Only a worker that died can hold the shared
lock for good, so only that ends the wait now.

The completeness check no longer refuses rows numbered past the target.
An append given a smaller target than the checkpoint holds is an append
question, and refusing it here both changed what append means and said
rows were missing when they were extra.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov found the branch: a blank line is skipped rather than counted
as a row that cannot be read, and nothing exercised it. An interrupted
write is exactly what leaves them, so reporting a damaged log there
would fail a run that lost nothing.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
A collector's values are merged over the output record after the index
has been written into it, and the merge lets the right-hand side win,
so a key called index replaced the number of the simulation the row
belonged to. Measured on a two-simulation run with a collector
returning 999: both rows were written as 999.

Every check on a finished run reads that field, num_of_loaded_sims
counts on it, and an import keys results by it, so a run with such a
collector was writing a log nothing downstream could read correctly.
It was quiet until the completeness check turned it into a refusal
that named the wrong cause.

Refused where the collector is handed over, alongside the existing
export_list clash, so it costs nothing and happens before a file is
opened. A collector under any other name is untouched, which has a
test of its own.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 force-pushed the bug/report-a-worker-that-fails-before-its-first-simulation branch from cad1281 to bb743e4 Compare August 25, 2026 15:58
Review practice on this repository is consistent about breadth: broad
exception handlers get asked about, and a guard that cannot fire gets
asked to go. Both applied here.

terminate and kill were wrapped against a process that dies between the
liveness check and the signal. multiprocess already swallows
ProcessLookupError there and retries once on OSError, so the wrapper
could not fire and is gone.

The three that remain say what they expect. A manager that has gone
away answers a proxy call with a broken connection or an EOF, a log
that cannot be written raises OSError, and a closed stream raises
OSError or ValueError. Anything else now reaches the caller.

One CHANGELOG line, since one pull request is one line here and the
workflow that would have written it has not run since RocketPy-Team#1112 (RocketPy-Team#1173).

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 force-pushed the bug/report-a-worker-that-fails-before-its-first-simulation branch from bb743e4 to 629cd87 Compare August 25, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants